I'm trying to follow the idea here. Is that idea that since the users vps won't be triggered otherwise, we need to manually trigger them to see if they approve a multitoken transfer? If so, shouldn't we pass in the verifiers set here and insert the necessary vps?

There are a couple of ways we could approach this and it'll probably be pretty involved (#432). At least for established accounts, we could force their VPs to be triggered to check the transaction, this is what vp_token does - if we go that route, we should then update vp_user to work with multitokens. For implicit accounts, this isn't an option, as we don't have implicit VPs (I think namadac transfer does not do auth for transfers between implicit addresses either, will double check this and open an issue if so). For addresses of internal accounts, we can just reject any transfers made to/from them or have special hardcoded logic as appropriate.

When I was experimenting in https://github.com/anoma/anoma-wasm-multitoken, I was thinking we could instead have tx.data signed by the sending account in such a way that the signature can be verified by the validity predicate. Then it would work like so:

• established accounts - we can look up the public key on chain of the Namada address which is sending the transfer, and use that to verify the signature
• implicit accounts - if we can't find a public key on chain, we must assume an implicit account (unless it's an internal account address which we can easily check) - in that case, we can "underive" the public key from the address

Ultimately the reader, tx_data and owner arguments passed in must be all that is needed to verify the transfer is authorized by the sender.

Implicit VPs are required for Namada launch, so I think triggering the necessary sender and receiver vps manually in this vp is the way to go.

Implicit VPs are required for Namada launch, so I think triggering the necessary sender and receiver vps manually in this vp is the way to go.

Implicit VP issue - #8

I will scope out #432 further. There are some issues I think with the implicit VP approach in general, not directly related to the eth bridge, I can write up. For now then we can at least get parity with what works for normal tokens (i.e. proper authorization for transfers sent from an established address). Authorization for transfers sent from implicit addresses does not currently work even for normal token transfers using namadac transfer, we will need to wait for #8 to be implemented

Since I've added these read_pre_value and read_post_value to the Ctx type in the bridge pool PR, let's just use those instead (just steal them). They return a simpler type (Option<T> instead of Result<Option<T>>) so it is simpler to handle.

We should share code functionality, though I'd rather we make an issue to update the Ctx API in main and then refactor everywhere at once. We shouldn't silently ignore errors though if they happen (i.e. we should use Result<Option<T>> rather than Option<T>).

These methods are in eth_bridge_integration now, so lets use them. But I agree, we shouldn't siliently ignore errors. It should be super fast to refactor the ctx methods to return the Result<Option<T>>

sure I can refactor there in our codebase quickly, I will make an issue before merging this PR to update the Ctx API in main. I need to look closer at why it returns Result<Option<T>> and not just Result<T> as at a glance it looks like these read_pre/read_post methods error if a key isn't present in storage, I would have thought in that case it would have defaulted to None and the errors would be reserved for things like e.g. rocksdb or OS-level errors.

hm why do we need the explicit borrow here, on ctx? was the compiler (or clippy) complaining about something?

It's because the trait this method comes from is implemented only for &Ctx and not Ctx

If there is a nice way to implement for both Ctx and &Ctx without too much duplication, I'd like to add it (though not in this PR) for ergonomics. (This StorageReader thing actually needs to be contributed back to main or may even be able to be replaced by the new native storage traits coming in soon)

good candidates for a proptest :D

will have a go at this (though not in this PR)